W3c trace context #331

pdimitra · 2021-07-29T11:21:14Z

This PR enables the forwarding and participating of the w3c trace context propagation.
The w3c trace context is going to be propagated for all HTTP requests
Additional changes occurred in this PR, are including moving the "asgi" span type from SDK to Registered.
The tracer test suite is passing all tests related to w3c trace context.

scorphus

Hi! 👋

Please consider my suggestions below.

instana/propagators/base_propagator.py

andrewslotin

Overall LGTM. I did not check the complete flow, let's make sure it passes the compatibility test suite first :)

instana/propagators/base_propagator.py

instana/propagators/binary_propagator.py

instana/w3c_trace_context/tracestate.py

instana/propagators/base_propagator.py

instana/w3c_trace_context/treceparent.py

hmadison

I think we need to drop Pika support untill we have a clear idea if we want to support AMQP header propagation. Outside of that, I'm happy to see this work get finished and I think making a dev0 release and testing internally should be our next steps.

instana/version.py

instana/w3c_trace_context/tracestate.py

tests/clients/test_pika.py

Co-authored-by: Pablo Aguiar <scorphus@gmail.com>

…strumentation, right now only for ASGI

… be applied for entry spans

Co-authored-by: Hunter Madison <hmadison@users.noreply.github.com>

…race context propagators and update all instrumentations to use the right one for headers extract/inject

andrewslotin

LGTM implementation-wise, but I'm not sure about the introduction of new propagator formats. Would it be possible to update the existing ones?

instana/tracer.py

instana/propagators/http_propagator_tc.py

instana/util/ids.py

instana/w3c_trace_context/traceparent.py

…efining the w3c trace context propagation

andrewslotin

I'd suggest we make W3C trace context support opt-out instead of opt-in. Otherwise LGTM

instana/tracer.py

instana/util/ids.py

andrewslotin

👍

scorphus reviewed Aug 3, 2021

View reviewed changes

instana/propagators/base_propagator.py Outdated Show resolved Hide resolved

instana/propagators/base_propagator.py Outdated Show resolved Hide resolved

instana/propagators/base_propagator.py Outdated Show resolved Hide resolved

pdimitra requested a review from andrewslotin August 5, 2021 12:11

andrewslotin suggested changes Aug 5, 2021

View reviewed changes

andrewslotin reviewed Aug 5, 2021

View reviewed changes

instana/w3c_trace_context/treceparent.py Outdated Show resolved Hide resolved

pdimitra marked this pull request as ready for review August 11, 2021 08:41

pdimitra requested review from andrewslotin and hmadison and removed request for andrewslotin August 11, 2021 11:21

hmadison approved these changes Aug 11, 2021

View reviewed changes

instana/version.py Outdated Show resolved Hide resolved

instana/w3c_trace_context/tracestate.py Outdated Show resolved Hide resolved

tests/clients/test_pika.py Outdated Show resolved Hide resolved

pdimitra and others added 21 commits August 13, 2021 12:05

adding traceparent,tracestate forwarding

cd6b5e4

adding the object for python2 compatibility

a877cf9

added validation of traceparent/tracestate headers and update methods

cb26817

fix the tests

56ecf4c

send updated w3c context headers

5d52645

adding span ctx calculation based on w3c trace context

2bd8c67

adding the w3c trace context relevant span properties

3412f35

adding docstrings and cleaning up

0977269

apply several fixes discovered through testing

c655fb2

add fixes discovered during testing

48a73cf

performing some optimisations

ce51332

adding unittests for traceparent and tarcestate

18859f9

Update instana/propagators/base_propagator.py

7e1218e

Co-authored-by: Pablo Aguiar <scorphus@gmail.com>

Update instana/propagators/base_propagator.py

6d8a9a1

Co-authored-by: Pablo Aguiar <scorphus@gmail.com>

Update instana/propagators/base_propagator.py

e7e1e8d

Co-authored-by: Pablo Aguiar <scorphus@gmail.com>

adding some unittests and apply some fixes based on review comments

c888613

adding unittests and apply some fixes based on review comments

9ee7db9

default binary w3c injection is disabled, it can be enabled as per in…

ff51688

…strumentation, right now only for ASGI

adding unittests

c844e8f

clean unused imports

d419507

document the binary_w3c_injection parameter

a7f2413

pdimitra and others added 15 commits August 13, 2021 12:08

add fixes for adjusting behavior to the testing tracer suite

ce333e1

fixing failing tests

c526ed8

moving the logic of the extra span attributes relative to w3c to only…

e82afb1

… be applied for entry spans

apply correlation properties fix

8dc635d

correct the env variable name

94dbcc6

fixing tests

8df08e7

apply fixes on w3c logic

ee4ed47

apply fixes on w3c logic

2fb89d7

small refactoring

a2503ee

adding the extra span attributes in SDK entry spans too

7b4632c

removed commented code block

1888d47

adding asgi in the registered spans

05954f3

fixing asgi switching span type related tests

37fe336

Update instana/version.py

f5416f5

Co-authored-by: Hunter Madison <hmadison@users.noreply.github.com>

decoupling the binary and http propagators into separate respective t…

335e632

…race context propagators and update all instrumentations to use the right one for headers extract/inject

pdimitra force-pushed the w3c-trace-context branch from aa7b01c to 335e632 Compare August 13, 2021 10:08

pdimitra requested a review from andrewslotin August 13, 2021 10:11

andrewslotin reviewed Aug 26, 2021

View reviewed changes

instana/tracer.py Outdated Show resolved Hide resolved

instana/propagators/http_propagator_tc.py Outdated Show resolved Hide resolved

instana/util/ids.py Show resolved Hide resolved

instana/w3c_trace_context/traceparent.py Outdated Show resolved Hide resolved

changes based on the review, propagators got merged, a parameter is d…

cd88045

…efining the w3c trace context propagation

andrewslotin suggested changes Sep 1, 2021

View reviewed changes

instana/tracer.py Outdated Show resolved Hide resolved

instana/tracer.py Outdated Show resolved Hide resolved

instana/util/ids.py Outdated Show resolved Hide resolved

pdimitra added 2 commits September 1, 2021 13:56

apply latest review changes

739f908

remove unneeded parameter

ef4a419

andrewslotin approved these changes Sep 1, 2021

View reviewed changes

pdimitra merged commit 2d4c9ca into master Sep 2, 2021

pdimitra deleted the w3c-trace-context branch September 2, 2021 04:05

W3c trace context #331

W3c trace context #331

Uh oh!

Conversation

pdimitra commented Jul 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scorphus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andrewslotin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hmadison left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andrewslotin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andrewslotin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andrewslotin left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pdimitra commented Jul 29, 2021 •

edited

Loading